Skip to content

Fix double free when an item's Drop panics - #60

Merged
agerasev merged 1 commit into
agerasev:masterfrom
tooson9010-spec:fix-panic-safety-double-free
Sep 13, 2026
Merged

agerasev merged 1 commit into
agerasev:masterfrom
tooson9010-spec:fix-panic-safety-double-free

Conversation

@tooson9010-spec

Copy link
Copy Markdown
Contributor

skip() and clear() drop the occupied items in a loop and advance the read
index only afterwards. If an item's Drop panics the index update never
happens, so the ring buffer still treats the destroyed items as live and its own
Drop — which calls clear() — destroys them again.

Reachable from safe Rust with any item type whose Drop can panic. Reported
earlier in #59.

Reproducer

The added test counts destructor calls on a 4-item buffer with one armed
Drop. On the current code clear destroys more items than exist:

clear: 5 drops for 4 items

With a heap-owning item type, AddressSanitizer reports attempting double-free
while the ring buffer's destructor runs.

The fix

Advance the read index one slot at a time, before each drop_in_place, so a
panicking Drop leaves the item outside the occupied range. clear() now
delegates to skip() so the ordering rule lives in one place.

This re-derives the occupied slices on every iteration, which costs more than
the single call the old loop made. If that matters here, a drop guard that keeps
the batch loop and fixes up the index on unwind would work too.

Regression test

src/tests/skip.rs gains a case that arms one item's Drop and asserts that no
item is destroyed more than once, for both clear and a partial skip. Plain
cargo test catches it, no sanitizer needed; the rest of the suite passes.

skip() and clear() dropped the occupied items in a loop and advanced the
read index only afterwards. If T::drop panics the index update never
happens, so the ring buffer's own Drop destroys the same items again.

Advance the read index one slot at a time, before each drop_in_place.
clear() now delegates to skip().
agerasev added a commit that referenced this pull request Sep 13, 2026
PR #60 advances the read index before drop_in_place, allowing a concurrent
producer to overwrite a slot while its destructor still accesses it. Its
per-item loop also makes clear consume newly inserted items rather than
finish after the original occupied snapshot.

Use an unwind guard to count started destructors and publish the read index
once, after the batch finishes or a panicking destructor has unwound. Keep
clear delegated to skip while preserving bounded batch traversal.

Extend regression coverage to actual clear calls, every panic position,
wrapped storage, owned/split/frozen consumers, buffer reuse, and concurrent
producer interactions. The new concurrency tests fail against the original
PR; the panic test fails against the original master.

Validation: scripts/test.sh and scripts/miri.sh pass; changed files pass
rustfmt and git diff --check.
agerasev added a commit that referenced this pull request Sep 13, 2026
Merge #60 with a follow-up correction
that keeps slots occupied until destruction finishes and commits removal
on both normal completion and panic unwinding.

Preserve bounded batch traversal and add panic/concurrency regressions.
Validated with scripts/test.sh and scripts/miri.sh.

Fixes #59.
@agerasev
agerasev merged commit 78f969c into agerasev:master Sep 13, 2026
1 check passed
@agerasev

Copy link
Copy Markdown
Owner

Thanks for the fix! And sorry for my late answer. I merged this with an unwind guard so slots become reusable only after destruction finishes.

@tooson9010-spec

Copy link
Copy Markdown
Contributor Author

@agerasev
Thanks for merging and releasing v0.5.2! Would you mind if I filed a RustSec
advisory for the affected versions, so users still on older versions have it
on record?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants